Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PK tests: use PSA to generate keypairs when USE_PSA is enabled #7393

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Apr 3, 2023

pk: add alternate function for keypair generation using PSA

Instead of using the legacy mbedtls_ecp_gen_keypair() which makes use of ECP's math, when USE_PSA_CRYPTO is enabled then the new function pk_genkey_ec() is used in test_suite_pk.

Depends on #7392
Resolves #7389

Gatekeeper checklist

  • changelog not required: internal change
  • backport not required: it's an improvement to support development-only work
  • tests not required: using already existing tests

@valeriosetti valeriosetti added enhancement needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Apr 3, 2023
@valeriosetti valeriosetti requested a review from mpg April 3, 2023 14:56
@valeriosetti valeriosetti self-assigned this Apr 3, 2023
@valeriosetti
Copy link
Contributor Author

valeriosetti commented Apr 3, 2023

@mpg TBH I'm not super sure about commit 632d3c5 since it seems to me that it limits the usage of gen_key app. However I don't know how much that application is used (at least not on test_suite_) so it might be acceptable as change.
If this is not the case then please let me know and I'll fix it ;)

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem with error management, and similar points about scope as previous PRs.

include/mbedtls/ecp.h Outdated Show resolved Hide resolved
tests/suites/test_suite_ecp.function Outdated Show resolved Hide resolved
tests/suites/test_suite_pk.function Outdated Show resolved Hide resolved
tests/suites/test_suite_pk.function Outdated Show resolved Hide resolved
programs/pkey/gen_key.c Outdated Show resolved Hide resolved
@valeriosetti valeriosetti force-pushed the issue7389 branch 3 times, most recently from 37a27db to 8ffeddc Compare April 4, 2023 14:54
@mpg mpg removed the needs-ci Needs to pass CI tests label Apr 5, 2023
mpg
mpg previously approved these changes Apr 5, 2023
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@valeriosetti
Copy link
Contributor Author

Rebased after most recent updates in #7392

@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Apr 7, 2023
@valeriosetti valeriosetti requested a review from mpg April 7, 2023 10:34
mprse
mprse previously approved these changes Apr 7, 2023
Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Instead of using the legacy mbedtls_ecp_gen_keypair() which makes
use of ECP's math, when USE_PSA_CRYPTO is enabled then the new
function pk_genkey_ec() is used in test_suite_pk.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
Copy link
Contributor Author

Rebased after #7392 has been merged into development

@valeriosetti valeriosetti requested a review from mprse April 11, 2023 07:18
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


status = psa_generate_key(&key_attr, &key_id);
if (status != PSA_SUCCESS) {
return MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only test but I think we should also destroy key if key generation fails in the middle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to me that this is the only point in that function for which there is a return instead of goto exit. However this seems ok to me since if psa_generate_key fails then there should be nothing to destroy. Other failures destroy the key. What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is all true. I'm not sure if we shouldn't also destroy key here. But looking at the description of psa_generate_key:

On success, an identifier for the newly created key.

It seems that current version is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I would expect psa_generate_key() to act in an atomic way: either it succeeds, or it leave the key slot unchanged, so that callers don't need to call destroy on failure. However I checked the documentation and it doesn't say explicitly. I checked other uses in the library, and they seem to only call destroy when generate succeeded but an error occurred later. So I think the code is fine as it is, but out of precaution I'll ask on the PSA Crypto API channel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However I checked the documentation and it doesn't say explicitly.

Well, I apparently didn't get enough sleep last night, because the documentation does say: "key On success, an identifier for the newly created key. PSA_KEY_ID_NULL on failure." So, we don't need to call destroy() on failure here. (It would however be legal, as psa_destroy_key(PSA_KEY_ID_NULL) is guaranteed to do nothing an return success.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a general principle that a PSA function doesn't affect the system state on failure. (It might leave its in-out arguments in a changed state on failure, however: multipart operation functions put the operation object in an error state when they fail.)

psa_generate_key and the other key creation functions are guaranteed to set the key_id output argument to 0 on failure. So it's guaranteed that calling psa_destroy_key(key_id) is always correct, but it's also guaranteed that it's a no-op if the creation function fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I should refresh the page more often when replying to comments :) I hadn't seen your replies when I wrote mines.

Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mpg mpg added approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Apr 11, 2023
@mpg mpg merged commit 6a327a5 into Mbed-TLS:development Apr 11, 2023
@valeriosetti valeriosetti deleted the issue7389 branch December 6, 2024 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports enhancement priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PK tests: use PSA to generate keypairs when USE_PSA is enabled
4 participants